Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove deprecated es.max-num-spans #2482

Merged

Conversation

BernardTolosajr
Copy link
Contributor

@BernardTolosajr BernardTolosajr commented Sep 17, 2020

Which problem is this PR solving?

Short description of the changes

  • Remove deprecated ES option --es.max-num-spans

@albertteoh
Copy link
Contributor

Thanks for quickly jumping on this, @BernardTolosajr. We're currently on 1.19.2 and we're targeting this for release 1.21.0 to give folks a chance to migrate over to --es.max-doc-count, so I'm not sure if we can merge this change just yet.

I've also restarted Crossdock and Unit Test jobs which appear to be caused by flaky tests.

@BernardTolosajr, I think you'll need to remove the code block here as well: https://github.com/jaegertracing/jaeger/blob/master/plugin/storage/es/options.go#L295

Basically, grep -ir maxNumSpans and remove accordingly.

@codecov
Copy link

codecov bot commented Sep 17, 2020

Codecov Report

Merging #2482 (3d1c18d) into master (27cb88f) will decrease coverage by 0.03%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2482      +/-   ##
==========================================
- Coverage   95.88%   95.84%   -0.04%     
==========================================
  Files         217      217              
  Lines        9647     9638       -9     
==========================================
- Hits         9250     9238      -12     
- Misses        328      330       +2     
- Partials       69       70       +1     
Impacted Files Coverage Δ
plugin/storage/es/options.go 100.00% <ø> (ø)
cmd/collector/app/server/zipkin.go 73.07% <0.00%> (-3.85%) ⬇️
cmd/query/app/static_handler.go 95.16% <0.00%> (-1.62%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 27cb88f...3d1c18d. Read the comment docs.

@BernardTolosajr
Copy link
Contributor Author

Noted on this one.

Copy link
Contributor

@albertteoh albertteoh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Copy link
Member

@yurishkuro yurishkuro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am blocking this for now to prevent being merged accidentally before v1.21

@objectiser objectiser mentioned this pull request Nov 12, 2020
@albertteoh
Copy link
Contributor

@BernardTolosajr it's a good time to merge this PR now that we've made a couple of releases. Can you please look at resolving the merge conflict? Let me know if you need help with this. Thanks.

@yurishkuro
Copy link
Member

yurishkuro commented Jan 22, 2021

fwiw, there are LOTS of other deprecated flags (#2738). But we should not clean them in a single PR since they usually require code changes, better to do one PR per flag.

./cmd/flags/admin.go:39:	healthCheckHTTPPortWarning = "(deprecated, will be removed after 2020-03-15 or in release v1.19.0, whichever is later)"
./cmd/flags/admin.go:40:	adminHTTPPortWarning       = "(deprecated, will be removed after 2020-06-30 or in release v1.20.0, whichever is later)"
./cmd/collector/app/builder_flags.go:45:	collectorHTTPPortWarning       = "(deprecated, will be removed after 2020-06-30 or in release v1.20.0, whichever is later)"
./cmd/collector/app/builder_flags.go:46:	collectorGRPCPortWarning       = "(deprecated, will be removed after 2020-06-30 or in release v1.20.0, whichever is later)"
./cmd/collector/app/builder_flags.go:47:	collectorZipkinHTTPPortWarning = "(deprecated, will be removed after 2020-06-30 or in release v1.20.0, whichever is later)"
./cmd/agent/app/reporter/flags.go:51:		flags.String(AgentTagsDeprecated, "", "(deprecated) see --"+agentTags)
./cmd/query/app/flags.go:42:	queryPortWarning        = "(deprecated, will be removed after 2020-08-31 or in release v1.20.0, whichever is later)"
./cmd/query/app/flags.go:43:	queryHOSTPortWarning    = "(deprecated, will be removed after 2020-12-31 or in release v1.21.0, whichever is later)"
./plugin/storage/cassandra/options.go:237:		"(deprecated) Jaeger will automatically detect the version of the dependencies table")
./plugin/storage/cassandra/options.go:241:		"(deprecated) Enable (or disable) host key verification. Use "+nsConfig.namespace+".tls.skip-host-verify instead")
./plugin/storage/es/options.go:186:		"(deprecated, will be removed in release v1.21.0. Please use "+nsConfig.namespace+".max-doc-count). "+
./pkg/config/tlscfg/flags.go:54:		flags.Bool(c.Prefix+tlsEnabledOld, false, "(deprecated) see --"+c.Prefix+tlsEnabled)
./pkg/config/tlscfg/flags.go:69:		flags.Bool(c.Prefix+tlsEnabledOld, false, "(deprecated) see --"+c.Prefix+tlsEnabled)
./pkg/config/tlscfg/flags.go:74:	flags.String(c.Prefix+tlsClientCAOld, "", "(deprecated) see --"+c.Prefix+tlsClientCA)

@mergify mergify bot dismissed yurishkuro’s stale review January 30, 2021 06:28

Pull request has been modified.

@BernardTolosajr
Copy link
Contributor Author

@albertteoh pushed fixed for the merge conflict. Let me know if i miss something. Thanks

@BernardTolosajr
Copy link
Contributor Author

@albertteoh should i also remove test for MaxNumSpansUsage?
https://github.com/jaegertracing/jaeger/blob/master/plugin/storage/es/options_test.go#L103

Thanks

@albertteoh
Copy link
Contributor

@albertteoh should i also remove test for MaxNumSpansUsage?
https://github.com/jaegertracing/jaeger/blob/master/plugin/storage/es/options_test.go#L103

Yes, please @BernardTolosajr.

Copy link
Member

@yurishkuro yurishkuro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please add an entry to changelog as a "breaking change"

Signed-off-by: Bernard Tolosa <bernardotolosajr@gmail.com>
Signed-off-by: Bernardo Tolosa <bernardotolosajr@gmail.com>
@BernardTolosajr BernardTolosajr force-pushed the remove-es-max-num-spans branch 2 times, most recently from fd09be5 to 900c550 Compare February 1, 2021 01:50
yurishkuro
yurishkuro previously approved these changes Feb 1, 2021
CHANGELOG.md Outdated

#### New Features
### New Features
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the original level 4 is correct

CHANGELOG.md Outdated
@@ -7,8 +7,9 @@ Changes by Version
### Backend Changes

#### Breaking changes
* Remove deprecated flags `es.max-num-spans` ([#2482](https://github.com/jaegertracing/jaeger/pull/2482),[@BernardTolosajr](https://github.com/BernardTolosajr))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* Remove deprecated flags `es.max-num-spans` ([#2482](https://github.com/jaegertracing/jaeger/pull/2482),[@BernardTolosajr](https://github.com/BernardTolosajr))
* Remove deprecated flag `--es.max-num-spans`, please use `--es.max-doc-count` ([#2482](https://github.com/jaegertracing/jaeger/pull/2482),[@BernardTolosajr](https://github.com/BernardTolosajr))

@mergify mergify bot dismissed yurishkuro’s stale review February 1, 2021 01:55

Pull request has been modified.

Signed-off-by: Bernardo Tolosa <bernardotolosajr@gmail.com>
@yurishkuro yurishkuro mentioned this pull request Feb 1, 2021
7 tasks
Copy link
Member

@yurishkuro yurishkuro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@BernardTolosajr Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove deprecated --es.max-num-spans flag in v1.21.0
3 participants